-
Notifications
You must be signed in to change notification settings - Fork 9.4k
magento/magento2#: Captcha. Improvement. Replace deprecated addError with addErrorMessage. #24340
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
magento/magento2#: Captcha. Improvement. Replace deprecated addError with addErrorMessage. #24340
Conversation
|
Hi @atwixfirster. Thank you for your contribution
For more details, please, review the Magento Contributor Guide documentation. |
1a49cf8 to
45de309
Compare
|
Why not replace all AddSuccess, AddError, and AddException calls to new format? |
ihor-sviziev
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @atwixfirster,
Could you review and fix failing static tests?
8469797 to
ee06632
Compare
ee06632 to
0a31fd4
Compare
|
@magento run all |
|
@magento combine 24334 24284 |
|
Hi @sidolov. Thank you for your request. I'm working on combining the pull requests for you |
|
@atwixfirster all pull requests have been successfully combined together:
|
dmytro-ch
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@atwixfirster , could you please also check the issue mentioned id the "child" PR #24334.
Thank you!
|
@magento run all tests |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @atwixfirster,
Could you review my comments?
Also there is some issues with tests for this PR, I think we can ignore these issues
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Isn’t it returns Address object? Looks like incorrect change
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, it returns Address object.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It’s better to keep original return statement there. Or it was incorrect?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, the origin return statement is correct.
Thanks, @ihor-sviziev
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why did you moved it above?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sort in alphabetical order ;) I can revert this change if you request it. Thanks
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It’s better to keep only needed changes in order to prevent conflicts, so please revert it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes. Now I understand this. Thank you
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why you move it above?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sort in alphabetical order ;)
I was wrong in this case... Thank you for notice
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It’s better to keep only needed changes in order to prevent conflicts, so please revert it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
d490c6e to
5de9fdf
Compare
|
Hi @ihor-sviziev, thank you for the review. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You made spelling mistake in HttpPosttActionInterface . Please, delete the extra letter t
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch, @engcom-Alfa !
Thank you!
engcom-Alfa
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You made spelling mistake in HttpPosttActionInterface . Please, delete the extra letter t
…with addErrorMessage. magento/magento2#: Replace deprecated Magento\Customer\Model\Customer::isConfirmationRequired method magento/magento2#: Replace deprecated addError, addSuccess, addException methods in Magento/Customer/Controller/Account/Confirmation.php
3281416 to
41a35bd
Compare
fixed |
|
Hi @ihor-sviziev, thank you for the review. |
|
✔️ QA Passed |
…ted addError with addErrorMessage. #24340
|
Hi @atwixfirster, thank you for your contribution! |
Description (*)
Pull request replaces deprecated
addErrormethod with a new one -addErrorMessagein theMagento_Captchamodule.Fixed Issues (if relevant)
Manual testing scenarios (*)
Questions or comments
Contribution checklist (*)
Thank you!